-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[docs] document use of theme.mixins.toolbar & <Toolbar /> when using Appbar variant fixed #17878
[docs] document use of theme.mixins.toolbar & <Toolbar /> when using Appbar variant fixed #17878
Conversation
No bundle size changes comparing a355648...832493a |
@oliviertassinari can you kindly point me in the right direction as to why |
Looks like the newly added |
You can ignore the check for now. It runs without being configured which is a bit unfortunate. Edit: Rebasing it will make it green though it isn't a requirement at the moment because it's experimental. |
@adeelibr Thanks! I'd imagine this is a pretty common footgun (only because I point it at myself all the time)! |
## Placement | ||
|
||
When using Appbar `variant="fixed"` you need to have extra space for the content to show below | ||
& not under. There are 2 ways to do it. Either use `theme.mixins.toolbar` like; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we tend to use & in the docs @mbrookes ?
## Placement | ||
|
||
When using Appbar `variant="fixed"` you need to have extra space for the content to show below | ||
& not under. There are 2 ways to do it. Either use `theme.mixins.toolbar` like; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That semicolon should be a colon right?
const useStyles = makeStyles(theme => ({ | ||
offset: { | ||
...theme.mixins.toolbar, | ||
flexGrow: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need flexGrow in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no flexGrow
is not needed, I'll update the PR
``` | ||
|
||
Or you can append `<Toolbar />` component after `<Appbar />` like shown in the example | ||
below. To prevent content from hiding under Appbar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of adding an plain example (to remove and source of doubt and to avoid having to dig in the following demo)?
} | ||
})) | ||
|
||
const App = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function App() {
?
This PR is related to #16844